Allow CORS mode to be optional #60
Conversation
src/Webflow.js
Outdated
| @@ -52,7 +52,7 @@ const responseHandler = (res) => | |||
| }); | |||
|
|
|||
| export default class Webflow { | |||
| constructor({ endpoint = DEFAULT_ENDPOINT, token, version = "1.0.0" } = {}) { | |||
| constructor({ endpoint = DEFAULT_ENDPOINT, token, version = "1.0.0", useCors = true } = {}) { | |||
There was a problem hiding this comment.
Maybe this should be a generic options = {} overload that the caller can use to provide anything to fetch 🤔 It might future proof it for other factors not considered.
There was a problem hiding this comment.
Hey @johnagan! Good idea. I've made the change but called it fetchOpts to be a bit more specific.
I think the only unfortunate side-effect to this approach is that CORS specifically requires you to use undefined, which is maybe unintuitive. I've added an example to the readme, though, to help offset this concern.
|
Thanks @jakemwood! I'm aligned with getting a fix for you. Made a suggestion on approach - let me know your thoughts. |
| @@ -76,6 +76,7 @@ export default class Webflow { | |||
| method, | |||
| headers: this.headers, | |||
| mode: "cors", | |||
| @@ -24,6 +24,9 @@ const Webflow = require('webflow-api'); | |||
| // Initialize the API | |||
| const api = new Webflow({ token: 'api-token' }); | |||
|
|
|||
| // Initialize the API without CORS mode | |||
| const apiNoCors = new Webflow({ token: 'api-token', fetchOpts: { mode: undefined }}); | |||
There was a problem hiding this comment.
I don't think this is a leftover. This is how you would accomplish the removal of cors mode with the fetch options override approach you suggested instead of the useCors: false approach from the first pass.
| @@ -76,6 +76,7 @@ export default class Webflow { | |||
| method, | |||
| headers: this.headers, | |||
| mode: "cors", | |||
| ...fetchOpts, | |||
There was a problem hiding this comment.
One thing about this approach is it would override the headers (including auth) if someone added a header entry.
There was a problem hiding this comment.
Hmmm. Could change the order:
mode: "cors",
...fetchOpts,
method,
headers: this.headers,
I think this would do the trick. Thoughts?
|
You're welcome to pushback on this, but my recommendation is to make a PR to override fetch options rather than focusing on disabling cors. Client Codeso I'd imagine something like this: const options = {
mode: undefined, // this is your use-case, but likely an edge case
headers: {
"User-Agent": "My Awesome Webflow App" // a more likely use-case for this feature
}
};
const webflow = new Webflow({ token: 'api-token', options });Changes to SDKPossibly add something like this to line 55. constructor({ endpoint = DEFAULT_ENDPOINT, token, version = "1.0.0", options = {} } = {}) {
if (!token) throw buildRequiredArgError("token");
this.responseWrapper = new ResponseWrapper(this);
this.endpoint = endpoint;
this.token = token;
// Enable header overrides
this.headers = Object.assign({
Accept: "application/json",
Authorization: `Bearer ${token}`,
"accept-version": version,
"Content-Type": "application/json"
}, options.headers);
this.authenticatedFetch = (method, path, data, query) => {
const queryString =
query && !isObjectEmpty(query) ? `?${qs.stringify(query)}` : "";
const uri = `${this.endpoint}${path}${queryString}`;
const opts = {
method,
mode: "cors",
...options,
headers, // set headers after blanket override
};
if (data) {
opts.body = JSON.stringify(data);
}
return fetch(uri, opts).then(responseHandler);
};
}I'm winging it on coding in comments here, but let me know what you think about this approach! |
Hello! We are trying to use the Webflow API with Cloudflare Workers. Unfortunately, their fetch API does not allow the usage of
corsmode. This seems to make sense, since there shouldn't be any cross-origin issues in a Node.JS environment, as there is no origin. However, I have added an optional parameter to the Webflow constructor (useCors) to ask if the user would like to use CORS or not. It defaults totrueto make this a non-breaking change. I've also updated the TypeScript definition to match.